467 feature request add dsts support #482

handsomejack-42 · 2024-04-15T11:23:03Z

No description provided.

handsomejack-42 · 2024-04-16T07:50:17Z

it's still in draft, but I would like some feedback @chlowell, if I'm taking this to the right direction. there's more than enough code already, I think I covered all the cases mentioned in the linked issue.

bgavrilMS · 2024-05-07T12:18:00Z

apps/confidential/confidential_test.go

 	localhost         = "http://localhost"
 	refresh           = "fake_refresh"
 	token             = "fake_token"
 )

 var tokenScope = []string{"the_scope"}

-func fakeClient(tk accesstokens.TokenResponse, credential Credential, options ...Option) (Client, error) {
-	client, err := New(fakeAuthority, fakeClientID, credential, options...)
+func fakeClient(tk accesstokens.TokenResponse, credential Credential, fAuthority string, options ...Option) (Client, error) {


Pls undo this name shortnening as it is harder to read the code.

bgavrilMS · 2024-05-07T12:23:56Z

apps/internal/oauth/ops/authority/authority.go

 		err = c.Comm.JSONCall(ctx, endpoint, http.Header{}, qv, nil, &resp)
 	}
 	return resp, err
 }

+func (c Client) DSTSInstanceDiscovery(ctx context.Context, authorityInfo Info) (InstanceDiscoveryResponse, error) {


Very surprised to see this, was under the impression that dSTS doesn't support instance discovery.

CC @victorm-hernandez

Correct, instance discovery is not supported.

bgavrilMS · 2024-05-07T12:25:30Z

apps/internal/oauth/ops/authority/authority_test.go

+		"tenant can't be common for AAD":        {authority: host + uuid1, tenant: "common", expectError: true},
+		"tenant can't be consumers for AAD":     {authority: host + uuid1, tenant: "consumers", expectError: true},
+		"tenant can't be organizations for AAD": {authority: host + uuid1, tenant: "organizations", expectError: true},
+		"can't override tenant for ADFS ever":   {authority: host + "adfs", tenant: uuid1, expectError: true},


Please add a test for ADFS where tenant is null / empty and where we expect to get back the original authority.

The field isn't used anywhere. Removing it simplifies the work on introducing authority abstraction. We might re-add it once we need it for anything.

…t readability Previously, the test results were printed as #0 #1 #2, which made it hard to navigate the failed test cases. Now, the test cases are annotated by their respective names. The names also make it more clear what is being unit-tested.

…ONCall Parsing the url, then setting qv.Encode() manually to u.RawQuery feel like an overcomplicated approach of creating url for http request. Use the native constructor, which enforces ctx to be passed into the request, and do simple sprintf for url + query.

This enables adding the same const for dSTS later in the process.

victorm-hernandez · 2024-05-22T21:15:15Z

apps/internal/oauth/ops/authority/authority.go

@@ -23,7 +23,8 @@ import (

 const (
 	authorizationEndpoint             = "https://%v/%v/oauth2/v2.0/authorize"
-	instanceDiscoveryEndpoint         = "https://%v/common/discovery/instance"
+	aadInstanceDiscoveryEndpoint      = "https://%v/common/discovery/instance"
+	dstsInstanceDiscoveryEndpoint     = "https://%v/dstsv2/common/discovery/instance"


https://%v/dstsv2/common/discovery/instance

We need some changes server side from dSTS before implementing this.

victorm-hernandez · 2024-05-22T21:26:21Z

apps/internal/oauth/ops/authority/authority.go

 		authorityType = ADFS
+	case "dstsv2":
+		if len(pathParts) != 3 {
+			return Info{}, errors.New(`dSTS authority must be an https URL such as "https://<authority>/dstsv2/<your tenant>"`)


dSTS doesnt support tenants, the value for the tenant is hard coded everywhere and should always be the same.

Lets change the error to reflect this. The tenant is

7a433bfc-2514-4697-b467-e0933190487f

hard-coded tenant + added validation

victorm-hernandez · 2024-05-22T21:29:07Z

apps/internal/oauth/resolvers.go

 		return fmt.Sprintf("https://%s/adfs/.well-known/openid-configuration", authorityInfo.Host), nil
+	} else if authorityInfo.AuthorityType == authority.DSTS {
+		resp, err := m.rest.Authority().DSTSInstanceDiscovery(ctx, authorityInfo)


DSTSInstanceDiscovery

Similar to ADFS, you can use a hard coded URL for dSTS. An example of this endpoint on dSTS

https://test-instance1-dsts.dsts.core.azure-test.net/dstsv2/7a433bfc-2514-4697-b467-e0933190487f/.well-known/openid-configuration

IMPORTANT! To connect to this endpoint you need to be on our VPN.

hard-coded the url

victorm-hernandez · 2024-05-22T21:31:05Z

apps/internal/oauth/ops/authority/authority_test.go

 	host := "https://localhost/"
-	for _, test := range []struct {
+
+	tests := map[string]struct {


tests

Please make sure to run this against real dSTS at least once.

The best way to do this is to create a sample project on the following repository

this test isn't actually firing any http requests, can you elaborate on what you mean by 'against real dSTS at least once'? I am not aware of any tests which I changed in the context of this PR are actually accessing anything e2e

I mean if you want me to manually validate that we can get a token from dSTS using this flow I can do that (I already did that a long time ago, but the set-up we used was deprecated by the recent dSTS crack-down on MS-corp based identities)

I guess that is why you are suggesting to 'build my own test dSTS' in C# and run it locally to test against? because at this point I can't see any easy way of how to call dSTS from SAW (I can hardly compile go there), so the only thing that comes to mind is some ev2 shell extension or something

I recommend moving this discussion outside of GitHub. Happy to cut a preview release for testing purposes. Agree that we should have some sort of manual validation, at least of some scenarios (client_credentials flow probably. I think user flows are a bit broken right now anyway)

victorm-hernandez · 2024-05-22T21:32:09Z

apps/confidential/confidential_test.go

+				ExtExpiresOn:  internalTime.DurationTime{T: time.Now().Add(1 * time.Hour)},
+				GrantedScopes: accesstokens.Scopes{Slice: tokenScope},
+				TokenType:     "Bearer",
+			}, cred, "https://fake_authority/dstsv2/fake_tenant")


fake_tenant

nit: tenant is always fixed on dSTS (7a433bfc-2514-4697-b467-e0933190487f)

victorm-hernandez · 2024-05-22T21:33:39Z

apps/confidential/confidential_test.go

+				t.Errorf("unexpected access token %s", tk.AccessToken)
+			}
+
+			// fail for another tenant


tenant

Given that dSTS has a single tenant, there is no need for this test.

I believe there still is a value in checking that we will fail the execution in case anyone tries to do WithTenant on dSTS flow

victorm-hernandez · 2024-05-22T21:36:07Z

apps/internal/oauth/ops/authority/authority.go

+	case ADFS:
+		return p, errors.New("ADFS authority doesn't support tenants")
+	case DSTS:
+		authority = "https://" + path.Join(p.AuthorityInfo.Host, "dstsv2", ID)


authority

Since the tenant ID should always be the same for dSTS, the correct behavior is to throw if it is not the expected GUID

7a433bfc-2514-4697-b467-e0933190487f

otherwise build the authority URL.

changed to always return an error if it hits this case, there are only three possible cases on WithTenant for dSTS flow that we care about

ID == "" -> returns identical AuthParams

ID == dSTS tenant -> returns identical AuthParams

neither of the above -> should return error

victorm-hernandez

🕐

This allows reusing the function for dSTS flow.

sonarcloud · 2024-05-29T09:31:15Z

Quality Gate passed

Issues
2 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

handsomejack-42 linked an issue Apr 15, 2024 that may be closed by this pull request

[Feature Request] Add dSTS support #467

Open

handsomejack-42 force-pushed the 467-feature-request-add-dsts-support branch 3 times, most recently from 5517451 to 83ca36e Compare April 15, 2024 16:02

handsomejack-42 requested a review from chlowell April 16, 2024 12:10

handsomejack-42 requested a review from bgavrilMS May 2, 2024 07:15

bgavrilMS reviewed May 7, 2024

View reviewed changes

bgavrilMS approved these changes May 7, 2024

View reviewed changes

bgavrilMS requested a review from victorm-hernandez May 7, 2024 12:31

handsomejack-42 removed the request for review from chlowell May 7, 2024 15:16

handsomejack-42 added 6 commits May 9, 2024 09:46

refactor: remove UserRealmURIPrefix from authority.Info

9575c6a

The field isn't used anywhere. Removing it simplifies the work on introducing authority abstraction. We might re-add it once we need it for anything.

test(oauth): add WithTenant test-cases to increase method test coverage

9113780

refactor(oauth): rename aad instance discovery endpoint const

5c855f3

This enables adding the same const for dSTS later in the process.

refactor(oauth): make WithTenant extensible with authority types

b71b8b8

handsomejack-42 force-pushed the 467-feature-request-add-dsts-support branch from 83ca36e to 35a4182 Compare May 9, 2024 08:33

handsomejack-42 marked this pull request as ready for review May 9, 2024 08:35

handsomejack-42 requested a review from rayluo as a code owner May 9, 2024 08:35

victorm-hernandez reviewed May 22, 2024

View reviewed changes

victorm-hernandez requested changes May 22, 2024

View reviewed changes

handsomejack-42 added 2 commits May 29, 2024 11:28

refactor(authority): use authority.ADFS instead of re-defined ADFS const

a725f82

refactor(confidential): fakeClient accepts authority as param

3b1540d

This allows reusing the function for dSTS flow.

handsomejack-42 force-pushed the 467-feature-request-add-dsts-support branch from 35a4182 to 5ee15d8 Compare May 29, 2024 09:29

feat(oauth): add support for dSTS authority type

99f29ff

handsomejack-42 force-pushed the 467-feature-request-add-dsts-support branch from 5ee15d8 to 99f29ff Compare May 29, 2024 09:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

467 feature request add dsts support #482

467 feature request add dsts support #482

handsomejack-42 commented Apr 15, 2024

handsomejack-42 commented Apr 16, 2024

bgavrilMS May 7, 2024

handsomejack-42 May 9, 2024

bgavrilMS May 7, 2024

victorm-hernandez May 22, 2024

bgavrilMS May 7, 2024

handsomejack-42 May 9, 2024

victorm-hernandez May 22, 2024 •

edited

Loading

handsomejack-42 May 29, 2024

victorm-hernandez May 22, 2024

handsomejack-42 May 29, 2024

victorm-hernandez May 22, 2024

handsomejack-42 May 29, 2024

victorm-hernandez May 22, 2024

handsomejack-42 May 29, 2024

handsomejack-42 May 29, 2024

bgavrilMS May 29, 2024

victorm-hernandez May 22, 2024 •

edited

Loading

handsomejack-42 May 29, 2024

victorm-hernandez May 22, 2024

handsomejack-42 May 29, 2024

victorm-hernandez May 22, 2024

handsomejack-42 May 29, 2024 •

edited

Loading

victorm-hernandez left a comment

sonarcloud bot commented May 29, 2024

467 feature request add dsts support #482

Are you sure you want to change the base?

467 feature request add dsts support #482

Conversation

handsomejack-42 commented Apr 15, 2024

handsomejack-42 commented Apr 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

victorm-hernandez May 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

victorm-hernandez May 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

handsomejack-42 May 29, 2024 • edited Loading

Choose a reason for hiding this comment

victorm-hernandez left a comment

Choose a reason for hiding this comment

sonarcloud bot commented May 29, 2024

Quality Gate passed

victorm-hernandez May 22, 2024 •

edited

Loading

victorm-hernandez May 22, 2024 •

edited

Loading

handsomejack-42 May 29, 2024 •

edited

Loading